Additional improvements to Store and Record API #1082

lbwexler · 2019-04-19T01:17:04Z

Restore 'raw' data property on record.
More maintainable handling of name collision
Don't require user to delete bad 'id' property on raw data. idSpec always wins.
Cleanup to left/right chooser

+ Restore 'raw' data property on record. + More maintainable handling of name collision + Don't require user to delete bad 'id' property on raw data. idSpec always wins. + Cleanup to left/right chooser

amcclain

Looks like some good cleanups / hardening of this part of our system.

I am still somewhat mystified as to how raw will be used, but if it's helpful it also seems very natural.

amcclain · 2019-04-19T19:17:56Z

data/Record.js

@@ -43,20 +42,31 @@ export class Record {
     * requiring children to also be recreated.)
     *
     * @param {Object} c - Record configuration
+     * @param {Object} c.data - data for constructing the record.
     * @param {Object} c.raw - raw data for record.


I know that apps aren't expected to construct records, but this is still a bit confusing here. How about:

c.data - data used to construct this record, pre-processed if applicable by store.processRawData(). c.raw - the same data, prior to any store pre-processing.

Would that be incrementally more clear?

amcclain · 2019-04-19T19:21:46Z

data/Record.js

@@ -43,20 +42,31 @@ export class Record {
     * requiring children to also be recreated.)
     *
     * @param {Object} c - Record configuration
+     * @param {Object} c.data - data for constructing the record.
     * @param {Object} c.raw - raw data for record.
     * @param {BaseStore} c.store - store containing this record.


Realizing that LocalStore defines idSpec, which means that's what you need to be passing in here. Yet BaseStore also declares interfaces that deal in Records/RecordSets.

We've discussed how we might want to unwind BaseStore, taking LocalStore -> Store and having it be the primary/parent store class. I dunno if we want to deal with that now.

Alternatively we could move idSpec to BaseStore, or adjust the docs here to punt a bit and deal with LocalStore only.

amcclain · 2019-04-19T19:22:00Z

data/Record.js


        this.id = id;
        this.store = store;
+        this.raw = raw;


We should add an @member declaration

amcclain · 2019-04-19T19:22:28Z

data/Record.js

+            if (name == 'id') return;
+            throwIf(
+                name in this,
+                `Field name "${name}" cannot be used for data. It is reserved as a top-level property of the Record class.`


Great that this check moved into this class.

amcclain · 2019-04-19T19:24:51Z

data/impl/RecordSet.js

+        let data = raw;
+        if (store.processRawData) {
+            data = store.processRawData(raw);
+            throwIf(!data, 'processRawData should return an object. If writing/editing, be sure to return a clone!');


Should we enforce processRawData always returning a new object? Is there a likely/important use case where we have processRawData making decisions to not edit some objects (and it would be terribly inefficient to force it to clone anyway)?

amcclain · 2019-04-19T19:27:16Z

desktop/cmp/leftrightchooser/LeftRightChooserModel.js

-
-            const rawRec = this._data.find(raw => raw === rec.raw);
-            rawRec.side = (rec.side === 'left' ? 'right' : 'left');
+            rec.raw.side = (rec.side === 'left' ? 'right' : 'left');


I'll be honest - I am not gonna dive into LRChooser code right now, but this seems very weird to be modifying the raw object.

amcclain · 2019-04-19T20:29:42Z

@haynesjm42 - Lee and I just talked this over a bunch more and he is going to press forward with some additional refactoring. Looking to collapse BaseStore+LocalStore -> Store + provide an efficient children getter on record (not directly related to this raw data change but will roll it all up into a bigger data package redo).

+ id should not be a field + LocalStore -> Store + Remove BaseStore

+ Implement record.children, record.allChildren + lint

lbwexler · 2019-04-22T12:16:45Z

This should be good to review and merge. Thanks to everyone for getting this in a good place. I think we now have a very tight, efficient TreeStore, with a very usable API.

data/impl/RecordSet.js

+ add rootRecords/allRootRecords getters

lbwexler · 2019-04-22T15:47:19Z

made a few tiny improvements -- john, can you take one more look ?

…

On Mon, Apr 22, 2019 at 11:29 AM John Haynes ***@***.***> wrote: ***@***.**** approved this pull request. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1082 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARTL2MVFTI35WKE2T4SK63PRXKTXANCNFSM4HHBKLFQ> .

haynesjm42

Looks good, just a couple minor comments.

Also it occurred to me that we have some logic in Grid which handles determining if the date is hierarchical or not, I wonder if we should move that into Store as a getter?

haynesjm42 · 2019-04-22T15:55:11Z

data/impl/RecordSet.js

+        records.forEach(r => {
+            const {parentId} = r;
+            if (parentId) {
+                let children = ret.get(parentId);


Looks like this could be const?

haynesjm42 · 2019-04-22T15:58:02Z

desktop/cmp/store/StoreCountLabel.js

@@ -58,7 +57,7 @@ export class StoreCountLabel extends Component {
        if (!store) return null;

        const includeChildren = withDefault(this.props.includeChildren, false),
-            count = includeChildren ? store.count : this.rootCount(store),
+            count = includeChildren ? store.count : store.rootRecords.length,


I think we've gone back and forth with this a few times, but should we have rootCount on Store for symmetry between the records and rootRecords properties? (I'm fine either way, we can always add it later if we see apps or other components with this kind of code)

+ Add efficient bulk remove functionality, streamline remove implementation

…tation

haynesjm42 · 2019-04-22T18:44:54Z

data/Store.js

     */
    @action
-    removeRecord(id) {
-        this._all = this._all.removeRecord(id);
+    removeRecords(ids) {


We could do a ids = castArray(ids)' here just to make it nice and clean for apps to remove a single record (something I tend to do a lot in inline editable grids)

amcclain · 2019-04-23T00:19:03Z

Great - excellent to see this landed! 🚀

+ Don't mutate data presented to loadData or updateData

aeefce7

+ Restore 'raw' data property on record. + More maintainable handling of name collision + Don't require user to delete bad 'id' property on raw data. idSpec always wins. + Cleanup to left/right chooser

lbwexler requested a review from haynesjm42 April 19, 2019 01:17

Lint and add ChangeLog record.

f0347b9

amcclain reviewed Apr 19, 2019

View reviewed changes

lbwexler added 5 commits April 19, 2019 17:39

Checkpoint

00f3475

+ id should not be a field + LocalStore -> Store + Remove BaseStore

Merge branch 'develop' into recordFixes

36f9a29

Enhnacement to ChangeLog

046b567

+ dataLastUpdated to lastUpdated

30742a7

+ Implement record.children, record.allChildren + lint

Changelog enhancements.

56ca575

lbwexler requested a review from amcclain April 22, 2019 12:13

lbwexler changed the title ~~+ Don't mutate data presented to loadData or updateData~~ Additional improvements to Store and Record API Apr 22, 2019

Whitespace and doc fix

b6472ad

haynesjm42 requested changes Apr 22, 2019

View reviewed changes

data/impl/RecordSet.js Outdated Show resolved Hide resolved

lbwexler added 2 commits April 22, 2019 09:53

Fix from John Haynes Code Review

ba8d102

Merge remote-tracking branch 'origin/recordFixes' into recordFixes

b28d628

haynesjm42 self-requested a review April 22, 2019 15:28

haynesjm42 approved these changes Apr 22, 2019

View reviewed changes

Compute childMap lazily -- often do not need!

8eb26a2

+ add rootRecords/allRootRecords getters

haynesjm42 reviewed Apr 22, 2019

View reviewed changes

lbwexler added 2 commits April 22, 2019 14:38

+ add rootCount/allRootCount getters for symmetry, efficiency

bb7b159

+ Add efficient bulk remove functionality, streamline remove implementation

+ Add efficient bulk remove functionality, streamline remove implemen…

a267633

…tation

haynesjm42 reviewed Apr 22, 2019

View reviewed changes

Suggestion from John Haynes CR

5c165c2

lbwexler merged commit f2fd604 into develop Apr 22, 2019

lbwexler deleted the recordFixes branch April 22, 2019 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Additional improvements to Store and Record API #1082

Additional improvements to Store and Record API #1082

lbwexler commented Apr 19, 2019

amcclain left a comment

amcclain Apr 19, 2019

amcclain Apr 19, 2019

amcclain Apr 19, 2019

amcclain Apr 19, 2019

amcclain Apr 19, 2019

amcclain Apr 19, 2019

amcclain commented Apr 19, 2019

lbwexler commented Apr 22, 2019

lbwexler commented Apr 22, 2019 via email

haynesjm42 left a comment

haynesjm42 Apr 22, 2019

haynesjm42 Apr 22, 2019

haynesjm42 Apr 22, 2019

amcclain commented Apr 23, 2019

Additional improvements to Store and Record API #1082

Additional improvements to Store and Record API #1082

Conversation

lbwexler commented Apr 19, 2019

amcclain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcclain commented Apr 19, 2019

lbwexler commented Apr 22, 2019

lbwexler commented Apr 22, 2019 via email

haynesjm42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcclain commented Apr 23, 2019